Conversation
There was a problem hiding this comment.
Pull request overview
Adds ergonomic helpers and documentation fixes aimed at improving real-world usage (session presets, safer frame utilities, and better debugging/logging), and fixes MkDocs asset paths for GitHub Pages.
Changes:
- Add session builder presets (
with_lan_defaults,with_internet_defaults,with_high_latency_defaults) and multipleDebugimpls for easier inspection/logging. - Add
Frameergonomic conversion/arithmetic helpers plus new structured error variants used by those helpers. - Update MkDocs config and docs asset paths; add new logo SVG assets.
Reviewed changes
Copilot reviewed 23 out of 27 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/time_sync.rs | Derive Hash for TimeSyncConfig to support hashing/caching use cases. |
| src/telemetry.rs | Strengthen trait impls/derives for TracingObserver (Copy/Eq/Hash) for ergonomic use. |
| src/sync_layer/game_state_cell.rs | Add GameStateCell::load_or_err() and Debug for GameStateAccessor; add tests. |
| src/sessions/sync_test_session.rs | Add custom Debug impl for SyncTestSession for better logging. |
| src/sessions/p2p_spectator_session.rs | Add custom Debug impl for SpectatorSession for better logging. |
| src/sessions/p2p_session.rs | Add custom Debug impl for P2PSession for better logging. |
| src/sessions/config.rs | Derive Hash for several config structs for hashing/caching use cases. |
| src/sessions/builder.rs | Add session preset helpers and corresponding tests. |
| src/rng.rs | Derive Hash for Pcg32 for hashing/caching use cases. |
| src/network/network_stats.rs | Derive Hash for NetworkStats for hashing/caching use cases. |
| src/network/chaos_socket.rs | Add PartialEq for ChaosConfig, derive Hash for ChaosStats, and add Debug for ChaosSocket. |
| src/lib.rs | Add multiple new Frame convenience APIs; document legacy From<usize> behavior. |
| src/hash.rs | Add equality derives for deterministic hashing types. |
| src/error.rs | Add new structured error variants/reasons used by new ergonomic APIs; add tests. |
| mkdocs.yml | Configure MkDocs logo/favicon to use new SVG assets. |
| examples/configuration.rs | Document and reference the new builder preset methods. |
| docs/user-guide.md | Fix logo asset path for MkDocs/GitHub Pages. |
| docs/migration.md | Fix logo asset path for MkDocs/GitHub Pages. |
| docs/index.md | Fix logo asset path for MkDocs/GitHub Pages. |
| docs/fortress-vs-ggrs.md | Fix logo asset path for MkDocs/GitHub Pages. |
| docs/contributing.md | Fix logo asset path for MkDocs/GitHub Pages. |
| docs/architecture.md | Fix logo asset path for MkDocs/GitHub Pages. |
| docs/assets/logo.svg | Add main SVG logo asset for docs/MkDocs. |
| docs/assets/logo-small.svg | Add small SVG logo asset for docs/MkDocs. |
| docs/assets/logo-banner.svg | Add banner SVG logo asset for docs/MkDocs. |
| CHANGELOG.md | Add Unreleased entries describing new APIs/derives/assets. |
| .gitignore | Ignore MkDocs build output directory (site/). |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 48 out of 54 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (2)
wiki/User-Guide.md:1313
- This newly-unignored Rust snippet defines the
Configtrait but referencesSerialize,DeserializeOwned,Hash, andDebugwithout importing or qualifying them. If someone runsscripts/verify-markdown-code.shacross the repo, this block will fail to compile; please make it self-contained by adding the neededusestatements (or fully-qualifying the paths).
wiki/Determinism-Model.md:394 - This code example uses
panic!()for desync handling. The project’s documentation guidelines call for zero-panic patterns in examples (prefer returningResult/ propagating errors or logging + clean shutdown). Please update this snippet to demonstrate non-panicking desync handling so users don’t copy/paste a panic-based pattern.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 53 out of 59 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (6)
wiki/User-Guide.md:1314
- This block is now tagged as
rust but it’s not a self-contained Rust snippet (e.g., `Serialize`, `DeserializeOwned`, `Hash`, `Debug` aren’t in scope here). If this is meant as illustrative API shape rather than copy/pasteable code, consider switching the fence totext, or add the minimaluseimports so the example compiles as shown.
wiki/User-Guide.md:1345 - This code block is now labeled as
rust, but it uses placeholders (`MyConfig`, `remote_addr`) and depends on the `tokio` feature (`#[tokio::main]`, `TokioUdpSocket`). If the intent is a copy/pasteable example, add the missing placeholder definitions and wrap tokio-specific parts in `#[cfg(feature = "tokio")]` (or use atext fence if it’s illustrative only).
wiki/User-Guide.md:1385 - This example calls
SpecViolation::to_json(), which is only available with thejsonfeature, but the snippet itself doesn’t show how to guard feature-gated code (and it’s now tagged asrust). Consider either adding a `#[cfg(feature = "json")]` wrapper around the `to_json()` usage (with a brief fallback example), or switching this block totext if it’s not meant to compile as-is.
wiki/User-Guide.md:505 - This block is tagged as
rust but references `game_state` without defining it in the snippet. If the goal is a copy/pasteable example, include a minimal `GameState` value in this same block (or switch to atext fence for illustrative fragments).
wiki/User-Guide.md:517 - This example is now labeled as
rust, but it depends on `game_state` being defined elsewhere (and uses `?`). For better copy/paste ergonomics, make the snippet self-contained (define `game_state` and any needed imports/types in this block) or change the fence totext if it’s intentionally partial.
src/lib.rs:1115 - The docs for
impl From<usize> for Framesay it “silently truncates values larger than i32::MAX”. Thevalue as i32cast can also wrap into negative values (e.g.i32::MAX as usize + 1becomesi32::MIN), not just “truncate above max”. Please update the wording to reflect the actual wrap/truncation behavior (and that it may yield negative/invalid frames).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 53 out of 59 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (3)
wiki/Determinism-Model.md:394
- This documentation example uses
panic!for desync handling. Since the project’s guidance emphasizes panic-free examples, it would be better to show a non-panicking pattern (e.g., logging + early return / signaling via an application error) so readers don’t copy a crash-on-desync approach.
examples/configuration.rs:108 - The comment says this is equivalent to
.with_lan_defaults(), but this builder usesTimeSyncConfig::responsive()whereaswith_lan_defaults()appliesTimeSyncConfig::lan(). Either switch toTimeSyncConfig::lan()or adjust the comment so it doesn’t imply exact equivalence.
// LAN/Local play - fast connections, minimal latency
// Equivalent to: .with_lan_defaults()
let lan_builder = SessionBuilder::<GameConfig>::new()
.with_num_players(2)
.unwrap()
.with_sync_config(SyncConfig::lan())
.with_protocol_config(ProtocolConfig::competitive())
.with_time_sync_config(TimeSyncConfig::responsive())
.with_input_delay(0)
examples/configuration.rs:146
- The comment says this is similar to
.with_high_latency_defaults(), butwith_high_latency_defaults()applies themobile()presets (and also adjusts the input queue), while this example uses thehigh_latency()/smooth()presets and doesn’t setInputQueueConfig::high_latency(). Consider either using the preset method directly in the example or adjusting the comment to avoid implying similarity to that preset.
// High-latency networks (80-200ms RTT)
// Similar to: .with_high_latency_defaults().with_max_prediction_window(12)
let high_latency_builder = SessionBuilder::<GameConfig>::new()
.with_num_players(2)
.unwrap()
.with_sync_config(SyncConfig::high_latency())
.with_protocol_config(ProtocolConfig::high_latency())
.with_time_sync_config(TimeSyncConfig::smooth())
.with_input_delay(4)
.unwrap()
| impl NonBlockingSocket<MyPeerId> for MyWebSocketTransport { | ||
| fn send_to(&mut self, msg: &Message, addr: &MyPeerId) { | ||
| // Serialize msg and send via WebSocket | ||
| let bytes = bincode::serialize(msg).unwrap(); | ||
| let Ok(bytes) = bincode::serialize(msg) else { return }; | ||
| self.send_to_peer(addr, &bytes); |
There was a problem hiding this comment.
This Rust snippet won’t compile under scripts/verify-markdown-code.sh (run in CI for docs/) because MyPeerId, send_to_peer, and drain_received_messages aren’t defined in the block. Either make the example self-contained with minimal type aliases/stubs, or change the fence language to text / add an intentional placeholder (... / // ...) so the verifier auto-skips it. Also consider logging serialization/deserialization failures instead of silently returning/dropping messages.
| let json = serde_json::to_string(&violation)?; | ||
|
|
||
| // Or use the convenience method | ||
| let json = violation.to_json().unwrap(); | ||
| let json_pretty = violation.to_json_pretty().unwrap(); | ||
| let json = violation.to_json()?; | ||
| let json_pretty = violation.to_json_pretty()?; |
There was a problem hiding this comment.
SpecViolation::to_json() / to_json_pretty() return Option<String> and are only available with the json feature, so using ? here is incorrect and will not compile. Consider gating the convenience-method portion with #[cfg(feature = "json")] and handling the Option (e.g., if let Some(json) = ...).
| ```rust | ||
| // Before | ||
| use fortress_rollback::Result; | ||
| fn my_function() -> Result<()> { ... } | ||
|
|
||
| // After (option 1: use the new name directly) | ||
| use fortress_rollback::FortressResult; | ||
| fn my_function() -> FortressResult<()> { ... } | ||
|
|
||
| // After (option 2: local alias if you prefer short names) | ||
| use fortress_rollback::FortressResult as Result; | ||
| fn my_function() -> Result<()> { ... } | ||
| ``` |
There was a problem hiding this comment.
This code block uses ... placeholders inside a rust fence, which looks like copy/pastable Rust but won’t compile. Consider switching the fence to text for before/after pseudo-code, or provide a compilable snippet (especially since this section is in the migration guide).
| let json = serde_json::to_string(&violation)?; | ||
|
|
||
| // Or use the convenience method | ||
| let json = violation.to_json().unwrap(); | ||
| let json_pretty = violation.to_json_pretty().unwrap(); | ||
| let json = violation.to_json()?; | ||
| let json_pretty = violation.to_json_pretty()?; |
There was a problem hiding this comment.
SpecViolation::to_json() / to_json_pretty() return Option<String> and are only available with the json feature, so using ? here is incorrect and will not compile. Consider gating this part with #[cfg(feature = "json")] and handling the Option (e.g., if let Some(json) = ...).
Description
✓ More ergonomic helpers to assist in real game usage
✓ Address banner not working in github pages
Type of Change
Checklist
Required
unwrap()in production codeexpect()in production codepanic!()ortodo!()Resultcargo fmt && cargo clippy --all-targetswith no warningscargo nextest runand all tests passIf Applicable
CHANGELOG.mdfor user-facing changesexamples/directoryTesting
Tests added/modified:
Manual testing performed:
Related Issues